Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make direct buffer allocation optional #39

Merged
merged 1 commit into from
Apr 3, 2017

Conversation

balthz
Copy link
Contributor

@balthz balthz commented Apr 3, 2017

This is a minor refactoring of all codecs that directly allocate Java ByteBuffers: instead of making a direct call to ByteBuffer#allocate(), the codec invokes a protected method makeBuffer(size). This allows a user to subclass the codec and override the method to customize the buffer allocation.

In particular, this enables users to use heap-buffers and/or buffer pooling. The latter is essential for reducing memory churn.

In a JVM benchmark (not included) on some real-world data, this refactoring did not decrease performance. (Notice that dropping final from some classes does not stop the JVM from inlining, such as in monomorphic callsites.)

This is a minor refactoring of all codecs that directly allocate Java `ByteBuffers`: instead of making a direct call to `ByteBuffer#allocate()`, the codec invokes a protected method `makeBuffer(size)`. This allows a user to subclass the codec and override the method to customize the buffer allocation.

In particular, this enables users to use heap-buffers and/or buffer pooling. The latter is essential for reducing memory churn.

In a JVM benchmark (not included) on some real-world data, this refactoring did not decrease performance. (Notice that dropping `final` from some classes does not stop the JVM from inlining, such as in [monomorphic callsites](https://shipilev.net/blog/2015/black-magic-method-dispatch/).)
@balthz
Copy link
Contributor Author

balthz commented Apr 3, 2017

@lemire Hey Daniel, I'm wondering whether you'd be interested in pulling in this change?

@coveralls
Copy link

Coverage Status

Coverage decreased (-0.7%) to 65.77% when pulling 35fdd72 on balthz:optional-direct-allocation into 26cb5f8 on lemire:master.

@lemire
Copy link
Member

lemire commented Apr 3, 2017

Thanks. Will review soon. Idea is sound.

@lemire
Copy link
Member

lemire commented Apr 3, 2017

Looks good.

@lemire lemire merged commit 5f31e2b into fast-pack:master Apr 3, 2017
@balthz
Copy link
Contributor Author

balthz commented Apr 3, 2017

Thanks a lot, @lemire!

@balthz
Copy link
Contributor Author

balthz commented Apr 3, 2017

The latest commit (3d46796) sounds as if this is about to be released – which would be fantastic for me, since I'm pulling from Maven Central. @lemire Is 0.1.11 underway or will you do this manually at some point? (I can pull in a JAR manually for now, so no rush.)

@lemire
Copy link
Member

lemire commented Apr 3, 2017

@balthz Yes. 0.1.11 has been released.

@hbf
Copy link

hbf commented Apr 3, 2017

Thanks again!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants